-
Notifications
You must be signed in to change notification settings - Fork 1.2k
A port of Blender's AgX to darktable #19026
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet tested, only a first pass on the code, just some QA checks.
@@ -25,3 +25,5 @@ output*png | |||
Brewfile.lock.json | |||
CMakeLists.txt.user | |||
workspace/ | |||
cmake-build-debug/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes in this file are probably superfluous.
@@ -0,0 +1,2286 @@ | |||
#include "bauhaus/bauhaus.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need the standard file header.
#include <pango/pangocairo.h> | ||
#include <stdlib.h> | ||
|
||
DT_MODULE_INTROSPECTION(2, dt_iop_agx_user_params_t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to keep the legacy here. I would start at 1 for the first integration. People having used the dev versions know that there won't be compatibility.
const char **description(dt_iop_module_t *self) | ||
{ | ||
return dt_iop_set_description(self, | ||
_("Applies a tone mapping curve.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No capital letters on default strings.
return hash; | ||
} | ||
|
||
static gboolean _agx_primaries_equal(gconstpointer a, gconstpointer b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add inline here?
GtkWidget *parent = self->widget; | ||
|
||
GtkWidget *settings_page | ||
= dt_ui_notebook_page(gui_data->notebook, N_("settings"), _("main look and curve settings")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=
should be above
gui_update(self); | ||
} | ||
|
||
static float _degrees_to_radians(float degrees) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline
+ const
Maybe adding this in common/math.c
(deg2rad) ?
dt_dev_add_history_item(darktable.develop, self, TRUE); | ||
} | ||
|
||
void commit_params(dt_iop_module_t *self, dt_iop_params_t *gui_params, dt_dev_pixelpipe_t *pipe, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one parameter per line
{ | ||
const dt_iop_agx_gui_data_t *gui_data = self->gui_data; | ||
|
||
if (picker == gui_data->black_exposure_picker) apply_auto_black_exposure(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style:
if (picker == gui_data->black_exposure_picker)
apply_auto_black_exposure(self);
else if (picker == gui_data->white_exposure_picker)
apply_auto_white_exposure(self);
else if (picker == gui_data->range_exposure_picker)
apply_auto_tune_exposure(self);
else if (picker == gui_data->basic_curve_controls_settings_page.curve_pivot_x_shift
|| gui_data->curve_tab_enabled && picker == gui_data->basic_curve_controls_curve_page.curve_pivot_x_shift)
{
...
dt_gui_presets_add_generic(_("smooth|punchy"), self->op, self->version(), &user_params, sizeof(user_params), 1, DEVELOP_BLEND_CS_RGB_SCENE); | ||
} | ||
|
||
void gui_cleanup(dt_iop_module_t *self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So can be removed.
An attempt to port Blender's AgX tone mapper to darktable, based on @EaryChow's https://github.com/EaryChow/AgX_LUT_Gen/blob/main/AgXBaseRec2020.py.
The tinting sliders are not implemented, no one seemed to require them at pixls, and I think darktable provides plenty of controls to implement that in other modules.
iop_profile.c
has been altered to make the output profile available as a source of base primaries. It is not essential to the module, but I think it would be useful to keep, e.g. for future gamut compression code. If accepted, maybe it'd make sense to update sigmoid, as well, where sRGB can currently be selected as a set of base primaries, too.(TBH, I'm scared witless. I feel like I'm in The Emperor's New Clothes, and I'm no emperor, just a clown, to be exposed to the world -- and being aware of the situation just makes it worse.)